Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adds shouldStripHeaderOnRedirectIfOnDifferentHostOnly option on Request #520

Merged
merged 4 commits into from
Oct 30, 2023
Merged

Adds shouldStripHeaderOnRedirectIfOnDifferentHostOnly option on Request #520

merged 4 commits into from
Oct 30, 2023

Conversation

meghfossa
Copy link
Contributor

@meghfossa meghfossa commented Oct 19, 2023 •
edited
Loading

Overview

This PR,

  • adds shouldStripHeaderOnRedirectIfOnDifferentHostOnly option to Request
  • modifies getRedirectedRequest signature to include original Request

Why?

Many stdlib in other language and ecosystem offer this functionality by default - refer to golang and python-requests for example. Potentially leaking headers to different host can lead to security incident. Refer to Python's CVE re this: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-20060

Ideally, shouldStripHeaderOnRedirect should have only stripped headers if the host differed, so without breaking change, this seems to be second best option.

Previous discussion on this topic: #300

Testing

  • I added few automated tests.

Remaining

  • Anything else is needed for contribution or release?

@meghfossa
Copy link
Contributor Author

Hi @snoyberg, let me know what I can do to get this to finish line!

@meghfossa meghfossa mentioned this pull request Oct 20, 2023
Closed
snoyberg
snoyberg requested changes Oct 24, 2023
View reviewed changes
Copy link
Owner

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable. Can you add a minor version bump and changelog entry? Also, please bump the @since comment to match the new version.

@meghfossa meghfossa requested a review from snoyberg October 28, 2023 22:43
@snoyberg snoyberg merged commit be63cb8 into snoyberg:master Oct 30, 2023
12 of 15 checks passed
@snoyberg
Copy link
Owner

Thanks! Released to Hackage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snoyberg snoyberg snoyberg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@meghfossa @snoyberg